refactor: add SQLite adapter to decouple from bun:sqlite#970
Merged
Conversation
Contributor
|
Contributor
Codecov Results 📊✅ 7012 passed | Total: 7012 | Pass Rate: 100% | Execution Time: 0ms 📊 Comparison with Base Branch
✨ No test changes detected All tests are passing successfully. ✅ Patch coverage is 85.25%. Project has 14087 uncovered lines. Files with missing lines (2)
Coverage diff@@ Coverage Diff @@
## main #PR +/-##
==========================================
+ Coverage 77.12% 77.24% +0.12%
==========================================
Files 320 321 +1
Lines 61947 61889 -58
Branches 0 0 —
==========================================
+ Hits 47773 47802 +29
- Misses 14174 14087 -87
- Partials 0 0 —Generated by Codecov Action |
c0b3adf to
972b864
Compare
Contributor
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 972b864. Configure here.
139bea9 to
4e51cb0
Compare
Introduce src/lib/db/sqlite.ts — a runtime-detecting adapter that provides a unified Database API for SQLite access. Under Bun it re-exports bun:sqlite directly (zero overhead). Under Node.js it wraps node:sqlite's DatabaseSync with the same .query()/.exec()/ .close()/.transaction() interface. This eliminates all direct bun:sqlite imports from src/ and test/, making the DB layer portable across runtimes without changing any call sites. Changes: - New: src/lib/db/sqlite.ts (adapter with runtime detection) - Updated: db/index.ts, schema.ts, migration.ts, utils.ts imports - Updated: 3 test files to import from adapter - Added: MIGRATION-PLAN.md documenting remaining migration steps
4e51cb0 to
c023df4
Compare
BYK
added a commit
that referenced
this pull request
May 20, 2026
## Summary Third step of the Bun → Node.js migration (follows #967, #970). Replaces all Bun-specific API calls in `src/` with Node.js equivalents. ### Changes **File I/O** (18 files): - `Bun.file(path).text()` → `readFile(path, "utf-8")` from `node:fs/promises` - `Bun.file(path).json()` → `JSON.parse(await readFile(path, "utf-8"))` - `Bun.file(path).exists()` → `access(path).then(() => true, () => false)` - `Bun.file(path).stat()` → `stat(path)` from `node:fs/promises` - `Bun.write(path, content)` → `writeFile(path, content)` from `node:fs/promises` - `Bun.write(dest, Bun.file(src))` → `copyFile(src, dest)` from `node:fs/promises` **Process/System** (9 files + 1 new): - `Bun.which()` → new `src/lib/which.ts` helper using `command -v` (POSIX) / `where` (Windows) - `Bun.spawn()` → `spawn()` from `node:child_process` with Promise-wrapped exit code - `Bun.spawnSync()` → `spawnSync()` from `node:child_process` - `Bun.sleep()` → `setTimeout()` from `node:timers/promises` **Utilities** (6 files): - `Bun.Glob` → `picomatch` (already a devDependency) - `Bun.randomUUIDv7()` → `uuidv7()` from `uuidv7` package - `Bun.semver.order()` → `compare()` from `semver` package ### What's NOT in this PR (Group D — separate follow-up) These Bun APIs in `bspatch.ts` and `upgrade.ts` require more careful handling: - `Bun.file().writer()` — streaming file writer (needs `fs.createWriteStream`) - `Bun.zstdCompress/DecompressSync` — zstd compression (needs `node:zlib` 22.15+) - `Bun.mmap()` — memory-mapped files (has existing fallback) - `Bun.CryptoHasher` — streaming hash (needs `crypto.createHash`) ### Test results All 7012 unit tests pass, 0 failures.
BYK
added a commit
that referenced
this pull request
May 20, 2026
…writer) (#986) ## Summary Fourth step of the Bun → Node.js migration (follows #967, #970, #984). Replaces the remaining Bun-specific APIs in `src/` — the "Group D" items that required more careful handling. ### Changes **`src/lib/bspatch.ts`** (rewritten): - `Bun.zstdDecompressSync()` → `zstdDecompressSync()` from `node:zlib` - `DecompressionStream('zstd')` → `createZstdDecompress()` from `node:zlib` piped through a Node Readable→Web ReadableStream bridge - `Bun.mmap()` → removed entirely; uses `readFile()` for both paths (copy-then-read and direct-read fallback) - `Bun.CryptoHasher("sha256")` → `createHash("sha256")` from `node:crypto` - `Bun.file(destPath).writer()` → `createWriteStream(destPath)` from `node:fs` **`src/lib/upgrade.ts`**: - `Bun.file(destPath).writer()` → `createWriteStream(destPath)` from `node:fs` **`src/lib/api/sourcemaps.ts`**: - `Bun.zstdCompress(buf, { level: 3 })` → `zstdCompress()` from `node:zlib` **`src/lib/telemetry/zstd-transport.ts`**: - Removed `globalThis.Bun.zstdCompress` dynamic access and `BunZstdHost` type - Replaced with direct `zstdCompress` from `node:zlib` (always available in Node 22.15+) - Removed belt-and-braces fallback (zstd can't disappear at runtime with `node:zlib`) - `hasZstdSupport()` now checks `typeof zstdCompressCb === "function"` directly **Tests updated** (`test/lib/telemetry/zstd-transport.test.ts`): - Removed 4 tests for `globalThis.Bun.zstdCompress` fallback paths (no longer applicable) - Replaced `Bun.zstdDecompress` with `zstdDecompressSync` in assertions ### Result **Zero non-comment `Bun.*` API calls remain in `src/`.** The only `bun:` reference left is the `require("bun:sqlite")` fallback in `sqlite.ts`, which will be removed when the test runner migrates to Vitest. All 7014 tests pass, 0 failures.
This was referenced May 20, 2026
BYK
added a commit
that referenced
this pull request
May 20, 2026
Consolidates fixes for review comments across PRs #967, #970, #984, #986. Changes: - Bump engines.node from >=22.12 to >=22.15 (zstd APIs require 22.15+, Node 20 is EOL). Update dist/bin.cjs runtime version check to match. - Simplify zstd imports: replace feature-detection dance with direct imports from node:zlib now that >=22.15 is guaranteed. - Fix spawn error handling in upgrade.ts: propagate error object to catch block so ENOENT/EBUSY detection works (was silently resolving with 1). - Fix sqlite.ts transaction(): wrap ROLLBACK in try/catch so original error is preserved if ROLLBACK itself fails. - Guard semver.compare calls in delta-upgrade.ts with semver.valid() to avoid throwing on invalid version strings. - Narrow catch in shell.ts addToGitHubPath to ENOENT only, re-throw other errors (EACCES, EPERM) instead of silently swallowing. - Add WriteStream backpressure handling in upgrade.ts streamDecompressToFile: check write() return value, await drain. - Add setup-node@v6 with Node 22 to E2E CI job. - Fix whichSync Windows CRLF: split on /\r?\n/ instead of \n. 7014 tests pass, 0 failures.
BYK
added a commit
that referenced
this pull request
May 20, 2026
Consolidates fixes for review comments across PRs #967, #970, #984, #986. Changes: - Bump engines.node from >=22.12 to >=22.15 (zstd APIs require 22.15+, Node 20 is EOL). Update dist/bin.cjs runtime version check to match. - Simplify zstd imports: replace feature-detection dance with direct imports from node:zlib now that >=22.15 is guaranteed. - Fix spawn error handling in upgrade.ts: propagate error object to catch block so ENOENT/EBUSY detection works (was silently resolving with 1). - Fix sqlite.ts transaction(): wrap ROLLBACK in try/catch so original error is preserved if ROLLBACK itself fails. - Guard semver.compare calls in delta-upgrade.ts with semver.valid() to avoid throwing on invalid version strings. - Narrow catch in shell.ts addToGitHubPath to ENOENT only, re-throw other errors (EACCES, EPERM) instead of silently swallowing. - Add WriteStream backpressure handling in upgrade.ts streamDecompressToFile: check write() return value, await drain. - Add setup-node@v6 with Node 22 to E2E CI job. - Fix whichSync Windows CRLF: split on /\r?\n/ instead of \n. 7014 tests pass, 0 failures.
BYK
added a commit
that referenced
this pull request
May 20, 2026
Consolidates fixes for review comments across PRs #967, #970, #984, #986. Changes: - Bump engines.node from >=22.12 to >=22.15 (zstd APIs require 22.15+, Node 20 is EOL). Update dist/bin.cjs runtime version check to match. - Simplify zstd imports: replace feature-detection dance with direct imports from node:zlib now that >=22.15 is guaranteed. - Fix spawn error handling in upgrade.ts: propagate error object to catch block so ENOENT/EBUSY detection works (was silently resolving with 1). - Fix sqlite.ts transaction(): wrap ROLLBACK in try/catch so original error is preserved if ROLLBACK itself fails. - Guard semver.compare calls in delta-upgrade.ts with semver.valid() to avoid throwing on invalid version strings. - Narrow catch in shell.ts addToGitHubPath to ENOENT only, re-throw other errors (EACCES, EPERM) instead of silently swallowing. - Add WriteStream backpressure handling in upgrade.ts streamDecompressToFile: check write() return value, await drain. - Add setup-node@v6 with Node 22 to E2E CI job. - Fix whichSync Windows CRLF: split on /\r?\n/ instead of \n. 7014 tests pass, 0 failures.
BYK
added a commit
that referenced
this pull request
May 20, 2026
## Summary Consolidates fixes for review comments across PRs #967, #970, #984, #986, superseding #988. ### Critical - **Bump `engines.node` to `>=22.15`** — `node:zlib` zstd APIs require 22.15+. Node 20 is EOL. Updated `dist/bin.cjs` runtime version check to match. - **Simplify zstd imports** — replaced feature-detection dance with direct `import { zstdCompress } from "node:zlib"` now that >=22.15 is guaranteed. ### High - **Fix spawn error handling in `upgrade.ts`** — `proc.on("error", () => resolve(1))` discarded the error object, making ENOENT/EBUSY detection dead code. Now properly rejects with the error. ### Medium - **Fix `sqlite.ts` ROLLBACK** — if ROLLBACK throws, the original transaction error was lost. Now wrapped in try/catch. - **Guard `semver.compare`** in `delta-upgrade.ts` with `semver.valid()` to avoid throwing on invalid version strings. - **Narrow catch in `shell.ts`** `addToGitHubPath` — only catch ENOENT, re-throw EACCES/EPERM. - **Add WriteStream backpressure** in `upgrade.ts` `streamDecompressToFile` — check `write()` return value, await `'drain'`. - **Add `setup-node@v6`** with Node 22 to E2E CI job. ### Low - **Fix `whichSync` Windows CRLF** — split on `/\r?\n/` instead of `\n` to strip trailing `\r` from `where.exe` output. ### Test results All 7014 tests pass, 0 failures. --------- Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

Summary
Second step of the Bun → Node.js migration (follows #967). Introduces a SQLite adapter layer that decouples the codebase from direct
bun:sqliteimports, making the database layer portable across runtimes.Changes
src/lib/db/sqlite.ts— SQLite adapter usingnode:sqlite(Node 22+)bun:sqlitewhennode:sqliteis unavailable (during Bun test runner transition only — fallback will be removed once tests migrate to Vitest)bun:sqlite's cached.query()when available, falls back tonode:sqlite's.prepare(); provides manual BEGIN/COMMIT/ROLLBACK wrapper fornode:sqlite(which lacks native.transaction())get()return value tonull(notundefined) for no-row results, matching codebase conventionDatabaseclass andSQLQueryBindingstypeisSchemaError()andisReadonlyError()inschema.ts— now match by error message content instead oferror.name === "SQLiteError", making auto-repair and readonly detection work undernode:sqlite(which throws plainError, notSQLiteError)db/index.ts,schema.ts,migration.ts,utils.tsfix.test.ts,telemetry.test.ts,schema.test.tslint-rules/no-manual-transactions.grit— excludesdb/sqlite.ts(manual transactions are necessary in the adapter fornode:sqlitecompatibility)MIGRATION-PLAN.mddocumenting remaining migration stepsDesign
Call sites continue using
.query(sql).get()/.all()/.run()anddb.transaction()exactly as before — no migration churn needed.Zero
bun:sqliteimports remain insrc/ortest/.